-
Notifications
You must be signed in to change notification settings - Fork 3
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: Block Verification Feature #414
Conversation
50fbeb9
to
fa09df7
Compare
932b207
to
bd9fb75
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did a fast first review, good progress, left some questions.
server/src/main/java/com/hedera/block/server/verification/BlockVerificationStatus.java
Outdated
Show resolved
Hide resolved
server/src/main/java/com/hedera/block/server/verification/StreamVerificationHandlerImpl.java
Show resolved
Hide resolved
...src/main/java/com/hedera/block/server/verification/hasher/ConcurrentStreamingTreeHasher.java
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@AlfredoG87 really good job!
This is my first pass through, I was focused mostly at looking at things on the surface level. Will need additional passes to get into the logic in depth, but the change is very big.
That being said, I comment you some nits/cleanup I found on the first pass.
I have not looked at the metrics and charts json changes as well as tests.
Here general comments:
-
I would not use
var
ever anywhere, even in test/loops etc. I think it makes things very unreadable and we should not be using that IMO. Not a blocker, but an opinion and a suggestion. -
I am sure I have missed places, but let's be adding the non-null annotation for the static code analysis.
-
Also I am sure I have missed some places, but lets always have a require non null checks on things that must not be null and we only do assignment (not calling a method on the target object). Lets fail early on these places. (Mostly in constructors or in methods where we need a non null object but we use it after executing a lot of logic before hand)
My focus on the next runs will be to get into and test the logic. Then tests (I presume there will be changes made, so I am hesitant to go to tests yet)
server/src/main/java/com/hedera/block/server/pbj/PbjBlockStreamServiceProxy.java
Outdated
Show resolved
Hide resolved
server/src/main/java/com/hedera/block/server/persistence/storage/PersistenceStorageConfig.java
Outdated
Show resolved
Hide resolved
server/src/main/java/com/hedera/block/server/persistence/storage/write/LocalBlockWriter.java
Show resolved
Hide resolved
server/src/main/java/com/hedera/block/server/verification/BlockVerificationStatus.java
Show resolved
Hide resolved
...main/java/com/hedera/block/server/verification/session/AbstractBlockVerificationSession.java
Outdated
Show resolved
Hide resolved
.../main/java/com/hedera/block/server/verification/session/BlockVerificationSessionFactory.java
Show resolved
Hide resolved
.../main/java/com/hedera/block/server/verification/session/BlockVerificationSessionFactory.java
Show resolved
Hide resolved
server/src/main/java/com/hedera/block/server/verification/signature/SignatureVerifier.java
Outdated
Show resolved
Hide resolved
1ad6423
to
898e9a5
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Another look, still no look at tests.
...src/main/java/com/hedera/block/server/verification/hasher/ConcurrentStreamingTreeHasher.java
Outdated
Show resolved
Hide resolved
server/src/main/java/com/hedera/block/server/verification/hasher/HashingUtilities.java
Show resolved
Hide resolved
server/src/main/java/com/hedera/block/server/verification/hasher/HashingUtilities.java
Show resolved
Hide resolved
server/src/main/java/com/hedera/block/server/verification/hasher/NaiveStreamingTreeHasher.java
Show resolved
Hide resolved
.../main/java/com/hedera/block/server/verification/session/BlockVerificationSessionFactory.java
Show resolved
Hide resolved
...main/java/com/hedera/block/server/verification/session/AbstractBlockVerificationSession.java
Outdated
Show resolved
Hide resolved
...main/java/com/hedera/block/server/verification/session/AbstractBlockVerificationSession.java
Outdated
Show resolved
Hide resolved
...main/java/com/hedera/block/server/verification/session/AbstractBlockVerificationSession.java
Outdated
Show resolved
Hide resolved
server/src/main/java/com/hedera/block/server/verification/signature/SignatureVerifier.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Much improved @AlfredoG87. It seems that some changes that you made (based on previous pr comments) were reverted for whatever reason?! (I believe this was unintentional) You can take a look at previous pr comments from previous passthroughs to see more reverted ones.
server/src/main/java/com/hedera/block/server/verification/VerificationConfig.java
Outdated
Show resolved
Hide resolved
server/src/main/java/com/hedera/block/server/verification/VerificationConfig.java
Outdated
Show resolved
Hide resolved
server/src/main/java/com/hedera/block/server/verification/VerificationInjectionModule.java
Show resolved
Hide resolved
server/src/main/java/com/hedera/block/server/verification/hasher/HashingUtilities.java
Show resolved
Hide resolved
server/src/main/java/com/hedera/block/server/verification/hasher/HashingUtilities.java
Show resolved
Hide resolved
server/src/main/java/com/hedera/block/server/verification/service/BlockVerificationService.java
Outdated
Show resolved
Hide resolved
...src/main/java/com/hedera/block/server/verification/service/BlockVerificationServiceImpl.java
Outdated
Show resolved
Hide resolved
server/src/main/java/com/hedera/block/server/verification/session/BlockVerificationSession.java
Outdated
Show resolved
Hide resolved
server/src/main/java/com/hedera/block/server/verification/signature/SignatureVerifierDummy.java
Outdated
Show resolved
Hide resolved
cdd9ba9
to
1c1928b
Compare
Did a forcce push in order to resolve a merge conflicts. |
With this many commits; perhaps a squash would be in order... |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Only part way through, but wanted to get these comments published.
A lot is suggestions or future considerations, but some are more immediately relevant.
...src/main/java/com/hedera/block/server/verification/hasher/ConcurrentStreamingTreeHasher.java
Outdated
Show resolved
Hide resolved
...src/main/java/com/hedera/block/server/verification/hasher/ConcurrentStreamingTreeHasher.java
Outdated
Show resolved
Hide resolved
...src/main/java/com/hedera/block/server/verification/hasher/ConcurrentStreamingTreeHasher.java
Outdated
Show resolved
Hide resolved
server/src/main/java/com/hedera/block/server/verification/hasher/HashingUtilities.java
Outdated
Show resolved
Hide resolved
server/src/main/java/com/hedera/block/server/verification/hasher/HashingUtilities.java
Show resolved
Hide resolved
...src/main/java/com/hedera/block/server/verification/hasher/ConcurrentStreamingTreeHasher.java
Show resolved
Hide resolved
...src/main/java/com/hedera/block/server/verification/hasher/ConcurrentStreamingTreeHasher.java
Show resolved
Hide resolved
...src/main/java/com/hedera/block/server/verification/hasher/ConcurrentStreamingTreeHasher.java
Show resolved
Hide resolved
...src/main/java/com/hedera/block/server/verification/service/BlockVerificationServiceImpl.java
Outdated
Show resolved
Hide resolved
...src/main/java/com/hedera/block/server/verification/service/BlockVerificationServiceImpl.java
Outdated
Show resolved
Hide resolved
c11b4a6
to
9545e93
Compare
server/src/main/java/com/hedera/block/server/verification/VerificationConfig.java
Outdated
Show resolved
Hide resolved
...src/main/java/com/hedera/block/server/verification/service/BlockVerificationServiceImpl.java
Outdated
Show resolved
Hide resolved
3de75a6
to
7e88367
Compare
server/src/main/java/com/hedera/block/server/persistence/storage/PersistenceStorageConfig.java
Outdated
Show resolved
Hide resolved
common/src/main/java/com/hedera/block/common/utils/MathUtilities.java
Outdated
Show resolved
Hide resolved
Next steps: Add metrics to measure time to hash. Signed-off-by: Alfredo Gutierrez <[email protected]> initial metrics dashboard and observations Signed-off-by: Alfredo Gutierrez <[email protected]> some simplifications and moving around code, also more logs for tests added. and temporary stuff. Signed-off-by: Alfredo Gutierrez <[email protected]> Some more improvements and moving things to async model. however I think I will create several implementations to the interfaces with some simple and synchronous and another one concurrent and asynchronous. Signed-off-by: Alfredo Gutierrez <[email protected]> clean-up halfway. Signed-off-by: Alfredo Gutierrez <[email protected]> major refactor, clean-up 80% Signed-off-by: Alfredo Gutierrez <[email protected]> added NoOp implementation for the VerificationService Signed-off-by: Alfredo Gutierrez <[email protected]> added javadoc to BlockVerificationService interface Signed-off-by: Alfredo Gutierrez <[email protected]> some more javadocs Signed-off-by: Alfredo Gutierrez <[email protected]> adding some extra exception handling. Signed-off-by: Alfredo Gutierrez <[email protected]> cleanup Signed-off-by: Alfredo Gutierrez <[email protected]> cleanup Signed-off-by: Alfredo Gutierrez <[email protected]> Fixing Existing Unit tests and removing an addition (requires org.checkerframework.checker.qual;) on module-info by accident Signed-off-by: Alfredo Gutierrez <[email protected]> Unit test for StreamingTreeHasher and BlockVerificationService and some refactor around BlockVerificationService Signed-off-by: Alfredo Gutierrez <[email protected]> VerificationInjectionModuleTest Unit test and spotless improvements Signed-off-by: Alfredo Gutierrez <[email protected]> More unit tests Signed-off-by: Alfredo Gutierrez <[email protected]> More unit tests, for BlockVerificationSessionSync Signed-off-by: Alfredo Gutierrez <[email protected]> More unit tests, for BlockVerificationSessionSync Signed-off-by: Alfredo Gutierrez <[email protected]> Adding UT Coverage for BlockVerificationSession related classes. Small improvements in other tests as well. Improvements to the flow of finalizeVerification thanks to issues surfaced during unit testing Signed-off-by: Alfredo Gutierrez <[email protected]> restoring original simulator properties file Signed-off-by: Alfredo Gutierrez <[email protected]> improvements on performance for ASYNC Signed-off-by: Alfredo Gutierrez <[email protected]> improvements for the dashboard and the config class so it logs the actual/final configuration values Signed-off-by: Alfredo Gutierrez <[email protected]> Added env mappings for new verification config properties Signed-off-by: Alfredo Gutierrez <[email protected]> Added some missing javadocs Signed-off-by: Alfredo Gutierrez <[email protected]> adding env config properties documentation Signed-off-by: Alfredo Gutierrez <[email protected]> Added remaining javadocs across the whole project Signed-off-by: Alfredo Gutierrez <[email protected]> style fixes Signed-off-by: Alfredo Gutierrez <[email protected]> convert sha384HashTag to final Signed-off-by: Alfredo Gutierrez <[email protected]> some extra tests and dashboard improvement, also update the dashboard on the chart deployment Signed-off-by: Alfredo Gutierrez <[email protected]> remove extension Signed-off-by: Alfredo Gutierrez <[email protected]> style fixes Signed-off-by: Alfredo Gutierrez <[email protected]> changing VerificationResult for SIGNATURE_INVALID a more descriptive one. instead of something more ambiguous why not something more explicit. Signed-off-by: Alfredo Gutierrez <[email protected]> added missing return statement Signed-off-by: Alfredo Gutierrez <[email protected]> adding Objects.requireNonNull checks, not sure if is really needed and not just redundant. in any case, improvement on existing class Signed-off-by: Alfredo Gutierrez <[email protected]> adding Objects.requireNonNull checks, not sure if is really needed and not just redundant. in any case, improvement on existing class Signed-off-by: Alfredo Gutierrez <[email protected]> removed ExecutorService CommonPool fulfiller for all executor requests on DI, and provided it only for the current verification session factory. Signed-off-by: Alfredo Gutierrez <[email protected]> fixing test after Signed-off-by: Alfredo Gutierrez <[email protected]> Refactor VerificationConfig to use Preconditions class to handle verifications, and also used MathUtilities to handle the actual verification. Signed-off-by: Alfredo Gutierrez <[email protected]> fixing test Signed-off-by: Alfredo Gutierrez <[email protected]> using long instead of Long Signed-off-by: Alfredo Gutierrez <[email protected]> PR Review changes Signed-off-by: Alfredo Gutierrez <[email protected]> PR Review changes Signed-off-by: Alfredo Gutierrez <[email protected]> style fix Signed-off-by: Alfredo Gutierrez <[email protected]> add Objects.requireNonNull on constructor Signed-off-by: Alfredo Gutierrez <[email protected]> Adding missing NonNull Signed-off-by: Alfredo Gutierrez <[email protected]> returning to how it was Signed-off-by: Alfredo Gutierrez <[email protected]> Simplifying Constructor logic Signed-off-by: Alfredo Gutierrez <[email protected]> Feedback and spotless updating the licence Signed-off-by: Alfredo Gutierrez <[email protected]> addressing more feedback from PR Review Signed-off-by: Alfredo Gutierrez <[email protected]> Improving and defining a log format. Signed-off-by: Alfredo Gutierrez <[email protected]> setting the logging.properties as part of the java args so it gets picked up before anything is logged. Signed-off-by: Alfredo Gutierrez <[email protected]> mapping the logging.properties file to the docker-compose stack, so is easy to change the properties once the container is created by changing the file on build/docker and restart the container. Signed-off-by: Alfredo Gutierrez <[email protected]> removed no longer needed field Signed-off-by: Alfredo Gutierrez <[email protected]> changing to the way it can be overriden using env variables Signed-off-by: Alfredo Gutierrez <[email protected]> adding non null to dummy class dependencies Signed-off-by: Alfredo Gutierrez <[email protected]> spotless apply changes Signed-off-by: Alfredo Gutierrez <[email protected]> add a missing @nonnull Signed-off-by: Alfredo Gutierrez <[email protected]>
… and solving conflicts Signed-off-by: Alfredo Gutierrez <[email protected]>
Signed-off-by: Alfredo Gutierrez <[email protected]>
Signed-off-by: Alfredo Gutierrez <[email protected]>
Signed-off-by: Alfredo Gutierrez <[email protected]>
Signed-off-by: Alfredo Gutierrez <[email protected]>
Signed-off-by: Alfredo Gutierrez <[email protected]>
Signed-off-by: Alfredo Gutierrez <[email protected]>
…oring an illegal state. Signed-off-by: Alfredo Gutierrez <[email protected]>
…oring an illegal state. Signed-off-by: Alfredo Gutierrez <[email protected]>
Signed-off-by: Alfredo Gutierrez <[email protected]>
Signed-off-by: Alfredo Gutierrez <[email protected]>
Signed-off-by: Alfredo Gutierrez <[email protected]>
Signed-off-by: Alfredo Gutierrez <[email protected]>
Signed-off-by: Alfredo Gutierrez <[email protected]>
Signed-off-by: Alfredo Gutierrez <[email protected]>
Signed-off-by: Alfredo Gutierrez <[email protected]>
aa6c04f
to
45d5941
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! Nice work.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice work, I left a few comments
server/src/main/java/com/hedera/block/server/verification/StreamVerificationHandlerImpl.java
Outdated
Show resolved
Hide resolved
server/src/main/java/com/hedera/block/server/verification/hasher/NaiveStreamingTreeHasher.java
Show resolved
Hide resolved
...src/main/java/com/hedera/block/server/verification/service/NoOpBlockVerificationService.java
Show resolved
Hide resolved
...main/java/com/hedera/block/server/verification/session/AbstractBlockVerificationSession.java
Outdated
Show resolved
Hide resolved
...rc/main/java/com/hedera/block/server/verification/session/BlockVerificationSessionAsync.java
Outdated
Show resolved
Hide resolved
.../main/java/com/hedera/block/server/verification/session/BlockVerificationSessionFactory.java
Show resolved
Hide resolved
server/src/main/java/com/hedera/block/server/verification/signature/SignatureVerifierDummy.java
Show resolved
Hide resolved
Signed-off-by: Alfredo Gutierrez <[email protected]>
Signed-off-by: Alfredo Gutierrez <[email protected]>
975cbc9
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Description:
New Classes:
verification
packageverification.hasher
packageverification.service
package:verification.session
pacakge:verification.signature
package:Other notable changes:
StreamVerificationHandlerImpl
as a new subscriber to theunverified ring buffer
.Related issue(s):
Fixes #375
Fixes #374
Fixes #376
Fixes #173
Fixes #423
Fixes #424
Notes for reviewer:
For the scenarios below, I use a 501 blocks sample with up-to-date (Dec-18-2024) very high TPS (10K).
The default implementation will be
ASYNC
that not only is faster, but is alsonon-blocking
Stats with
SYNC
implementation:Stats with
ASYNC
implementation:Checklist